- 
                Notifications
    
You must be signed in to change notification settings  - Fork 76
 
[FIX] Workflow saving: fix cancel, add ows extension by default #330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIX] Workflow saving: fix cancel, add ows extension by default #330
Conversation
486d197    to
    094a761      
    Compare
  
    | 
           Could be done in that way, but there can happen weird behaviour: 
 There is another bug:orange-canvas-core 0.2.6
  | 
    
| 
           Second commit fix mentioned bug.  | 
    
| 
           Thank you for this PR, and sorry that we took so long to look into it. I especially appreciate the fix for the correct treatment of clicking on the "Cancel" in the dialog. That was a nasty bug. I like the idea of keeping the .ows extension, but now this PR introduced a new bug. Let's assume a file "a.ows" already exists. Then, the user saves their workflow into "a". Now, the code silently add ".ows" and this the user is not notified that they are overwriting an existing file. So that silent addition of ".ows" is dangerous. We did attempt to handle this in the Save widget and the solution in owsavebase.py (from biolab/orange3) was to manually put up an additional dialog if needed. We could do something similar here. So the crux is this: owsavebase.py handles different platforms separately, but I think the above "post-processing" of the dialog results should work with all platforms. Could you try a similar patch?  | 
    
| 
           Please also rebase to master. If you perhaps don't have time to work on this PR, please tell us, so that we can add the check ourselves. Thank you!  | 
    
2ab8e77    to
    05b88a3      
    Compare
  
    | 
           Yup, I know about that weird behaviour, but in that moment I cannot figure out where is the best place to do this. So what I was thinking, was compromise. 😎 Thanks for this override snippet - added.  | 
    
| # Enforcing ".ows" extension during saving file. | ||
| # see .../biolab/orange-canvas-core/pull/330 | ||
| filename = filename if str(filename).endswith('.ows') else f"{filename}.ows" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is, what is reverted
05b88a3    to
    25a0d32      
    Compare
  
    | 
           /rebase  | 
    
| 
           I don't get one thing. When I run tests at my machine I have to still accept manually dialog with "override", even though the settings:  Maybe here will not ask, but if yes - any ideas?  | 
    
| 
           Confirmation comes a few lines later in the code, where it calls   | 
    
3f5dd9d    to
    feff7b5      
    Compare
  
    | 
           @markotoplak, after you approve the PR (please also add a comment so I get an email), I can update the translations and merge.  | 
    
          Codecov Report❌ Patch coverage is  
 Additional details and impacted files@@           Coverage Diff           @@
##           master     #330   +/-   ##
=======================================
  Coverage   76.07%   76.08%           
=======================================
  Files         102      102           
  Lines       21326    21321    -5     
=======================================
- Hits        16224    16222    -2     
+ Misses       5102     5099    -3     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
  | 
    
| 
           I don't like this mechanism. There is enough to write name as "title.o" to skip this at all and save file which can't be found... Anyway that fix of "saving at cancel" is more important.  | 
    
This is one solution among two I think. The second one is change visibility of files the user can choose from when opening saved workflow. The first is simpler I guess and it seems more reasonable. Possibility about change type of `filename` when passed as `pathlib.Path`, but this operation here is safe. Casting will not raise exception when `filename` is `pathlib.Path` type indeed.
- reverts commit 4f02bf8. - Add question MessageBox when overriding file - changed place when `filename` is changed to `filename`.ows in compare to reverted commit.
- Add suffix to filename for file created by tempfile to be compatible with enforcing 'ows' extension. - Mocked `exec` method for QFileDialog now return True, because it was never reach out branch `if dialog.exec():`.
I haven't seen it when fixing tests... defaultSuffix Previously solution is bad, because there will pop-up two MessageBox, second when FileDialog is already closed.
6a7efdc    to
    8141870      
    Compare
  
    8141870    to
    abe7ba9      
    Compare
  
    | 
           For the sake of clarity, to be equal with master I did: 
  | 
    
| 
           I like your last solution and it seems to work for me! Thanks for taking the time and effort to produce something better than we suggested.  | 
    
I catched myself on doing that twice, when rename workflow file and cannot find it because of extension.
This change is about it. Add extension to filename if there is not.
This is one solution among two I think.
The second one is change visibility of files the user can choose from when opening saved workflow.
The first is simpler I guess and it seems more reasonable.
There is possibility about change type of
filenamewhen passed aspathlib.Path,but this operation is safe in this place. Casting will not raise exception when
filenameispathlib.Pathtype indeed.I changed it, because in my first commit
basenamewas from originalfilename, so it can misguide.Used Orange3 in version: 3.39